fix: check container runtime is available in stop and restart - #1036
Conversation
| } | ||
|
|
||
| pub fn logs(&self, instance_name: &str, follow: bool) -> Result<()> { | ||
| Self::check_available(self.runtime)?; |
There was a problem hiding this comment.
When helix logs runs with Docker installed but its daemon stopped, check_available starts Docker Desktop, Colima, or the Docker service and waits for startup. This makes a read-only logs command mutate runtime state instead of promptly reporting that the runtime is unavailable.
There was a problem hiding this comment.
Pull request overview
This PR aligns helix stop, helix restart, and helix logs with helix start by preflighting the configured container runtime so missing-runtime spawn failures are routed through the existing “not installed” error (including the Podman hint) instead of surfacing as a bare OS error.
Changes:
- Add
Self::check_available(self.runtime)?toLocalRuntime::stop(). - Add
Self::check_available(self.runtime)?toLocalRuntime::restart(). - Add
Self::check_available(self.runtime)?toLocalRuntime::logs().
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pub fn restart(&self, instance_name: &str, config: &LocalInstanceConfig) -> Result<()> { | ||
| Self::check_available(self.runtime)?; | ||
| if config.storage.is_disk() || config.storage.is_s3() { | ||
| return self.run_detached(instance_name, config); | ||
| } |
| } | ||
|
|
||
| pub fn logs(&self, instance_name: &str, follow: bool) -> Result<()> { | ||
| Self::check_available(self.runtime)?; |
292c32e to
aafc8dd
Compare
run_detached and run_foreground both call check_available before they touch the runtime, which routes a missing binary through not_installed_error and produces an actionable message. stop and restart spawned the binary directly, so on a host without the configured runtime they failed with a bare os error 2 that does not name what was missing. restart checks only on the branch that spawns the runtime itself; the disk and s3 paths return early into run_detached, which already checks.
aafc8dd to
cb749ed
Compare
|
both fair, and the second one caught an inconsistency in my own reasoning. fixed in cb749ed. on the redundant probe in restart: the check now sits after the disk and s3 early return, so it only guards the branch that spawns the runtime itself. those paths fall through to run_detached, which already checks, so there is no longer a double probe. on logs auto-starting the daemon: you are right and i was applying two different standards. i had already left that does leave |
helix logs and helix status shell out to the container runtime without checking that it can be spawned, so on a host that has podman but not docker they fail with a bare "No such file or directory (os error 2)". helix start and helix stop on the same host report that Docker is not installed and suggest setting container_runtime = "podman" instead. #1036 gave stop and restart that message by calling check_available. logs and status were deliberately left out, because check_available auto-starts the daemon and that is the wrong side effect for a read-only command. This keeps that constraint rather than reversing it, and maps the spawn failure through not_installed_error, the helper check_available already uses to build the message. It runs only when the process could not be spawned at all, so nothing is probed and nothing is started. A runtime that is installed but not running still spawns and returns a non-zero status, so the existing error paths are unchanged. The originating os error is preserved as the cause line.
on a host that has podman but not docker, `helix logs` and `helix status` both die with `Failed to read logs for helix-x-dev: No such file or directory (os error 2)`. `helix start` and `helix stop` on that same box give you `Docker is not installed` plus a help line telling you to set `container_runtime = "podman"` in helix.toml. same machine, same missing binary, two completely different errors. #1036 fixed stop and restart by calling `check_available` first. logs and status were left out of that one on purpose, since `check_available` auto starts the daemon and doing that as a side effect of a read only command is wrong. that reasoning still holds, so this doesn't touch `check_available`. instead it routes the spawn failure through `not_installed_error`, which is the helper `check_available` already uses to build that message and the only place it was ever called from. it only fires when the process could not be spawned at all, so nothing gets probed and nothing gets started. if the binary is there and the daemon is just down, `docker logs` still spawns fine and returns non zero, so the existing error paths are untouched. two lines of src, the rest is the test. before ``` Failed to inspect helix-repro1036-dev: No such file or directory (os error 2) Failed to read logs for helix-repro1036-dev: No such file or directory (os error 2) ``` after ``` error: Docker is not installed (`docker` not found on PATH) │ No such file or directory (os error 2) = help: Podman is installed — set `container_runtime = "podman"` under [project] in helix.toml to use it instead, or install Docker. ``` the underlying os error is kept as the cause line, so nothing is lost. the test points `HELIX_TEST_CONTAINER_RUNTIME_BIN` at a path that doesn't exist, which is the same spawn failure a missing docker gives, and checks that logs, status and stop all name the runtime and all still carry the os error. stop is in that loop on purpose, so if someone changes one of the three later the other two don't quietly drift again. `prune_instance` reaches the same bare error through `remove_container`. left it out to keep this focused, happy to do it separately if you want it. tests ``` cargo test -p helix-cli --test runtime_commands cargo clippy -p helix-cli --all-targets -- -D warnings cargo fmt --all -- --check ``` <!-- greptile_comment --> <h3>Greptile Summary</h3> The PR improves missing-container-runtime diagnostics for the read-only logs and status commands without probing or starting the daemon. - Routes logs and status spawn failures through the shared runtime-not-installed diagnostic. - Adds a missing-runtime fixture and regression coverage across logs, status, and stop. - The new mapping also labels non-missing-binary spawn failures as installation failures. <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | crates/cli/src/local_runtime.rs | Improves missing-runtime errors for logs and status, but unconditionally misclassifies every spawn failure as a missing binary. | | crates/cli/tests/runtime_commands.rs | Adds focused regression coverage confirming logs, status, and stop identify a missing Docker executable while retaining the OS error. | | crates/cli/tests/support/mod.rs | Adds a fixture helper that reliably directs runtime commands to a nonexistent executable. | </details> <sub>Reviews (1): Last reviewed commit: ["fix: name the missing container runtime ..."](d20a3c7) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=57831448)</sub> > Greptile also left **1 inline comment** on this PR. <!-- /greptile_comment -->
helix startalready checks the container runtime is there before it does anything. if docker is configured but not installed, it stops with an error that names the missing binary and tells you podman is available instead. stop and restart skip that check and spawn the binary directly, so on the same machine they fail with a bare os error.on a host with podman and no docker, using the
container_runtime = "docker"thathelix initwrites by default:os error 2 never says which binary was missing, so there is nothing in it to act on. you get it on first run too, because init writes docker into helix.toml without looking at what is installed, so a podman user hits this before anything else works.
the fix is already in the file. run_detached and run_foreground both open with
Self::check_available(self.runtime)?, and check_available is what routes a spawn failure through not_installed_error to produce the podman hint above. this adds the same line to stop and restart so they land on the error that was already written for this case. no new error text and no new behaviour, two commands just stop bypassing it.in restart the check sits after the disk and s3 early return, not before it. those paths hand off to run_detached, which already checks, so checking first would probe the daemon twice.
stop and restart are the two commands here that go on to change container state, which is why auto-starting a stopped daemon is reasonable for them, the same as it already is for start.
logsandstatushave the same bare error but they are read only, andcheck_availablewill start docker desktop or colima when the binary is present but the daemon is down. starting a daemon as a side effect of reading logs is not something i wanted to slip into this pr. both would be better served by a preflight that checks the binary is installed without auto-starting, which is a different change. happy to send it if you want it.tested on wsl2, ubuntu 24.04 arm64, podman 4.9.3, docker not installed. after the change both print the same error as start, and with podman configured the normal lifecycle still works: start, status, query, restart, query again, logs, stop.
the typescript_runtime suite needs
npm ciinsdks/typescriptand node on PATH before it will pass; with those in place it is green here too.